Skip to content

[dotnet] [bidi] Separate empty and base result#15593

Closed
RenderMichael wants to merge 1 commit intoSeleniumHQ:trunkfrom
RenderMichael:empty-command
Closed

[dotnet] [bidi] Separate empty and base result#15593
RenderMichael wants to merge 1 commit intoSeleniumHQ:trunkfrom
RenderMichael:empty-command

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Apr 8, 2025 •
edited by qodo-code-review bot
Loading

User description

💥 What does this PR do?

The EmptyResult does two different things: act as a base type for all BiDi results, and act as a void result. We can separate these two responsibilities to two different types, leading to clearer code. Additionally, we can rest assured that any EmptyResult we get is not non-empty result without at runtime.

PR Type

Enhancement, Other

Description

  • Replaced EmptyResult with BiDiResult as the base type for BiDi command results.

  • Refactored multiple command result classes to inherit from BiDiResult.

  • Updated serialization and deserialization logic to accommodate the new BiDiResult type.

  • Improved code clarity by separating void results (EmptyResult) from the base result type.

Changes walkthrough 📝

Relevant files
Enhancement
25 files
Broker.cs
Update command execution to use `BiDiResult`                         
+6/-6     
Command.cs
Introduce `BiDiResult` and refactor command base                 
+5/-2     
BiDiJsonSerializerContext.cs
Update JSON serialization for `BiDiResult`                             
+1/-1     
InputOriginConverter.cs
Adjust serialization logic for shared references                 
+3/-4     
Message.cs
Update message success handling to use `BiDiResult`           
+1/-1     
GetClientWindowsCommand.cs
Refactor `GetClientWindowsResult` to inherit `BiDiResult`
 +1/-1     
GetUserContextsCommand.cs
Refactor `GetUserContextsResult` to inherit `BiDiResult` 
+1/-1     
UserContextInfo.cs
Update `UserContextInfo` to inherit `BiDiResult`                 
+1/-1     
CaptureScreenshotCommand.cs
Refactor `CaptureScreenshotResult` to inherit `BiDiResult`
 +1/-1     
CreateCommand.cs
Update `CreateResult` to inherit `BiDiResult`                       
+1/-1     
GetTreeCommand.cs
Refactor `GetTreeResult` to inherit `BiDiResult`                 
+1/-1     
LocateNodesCommand.cs
Update `LocateNodesResult` to inherit `BiDiResult`             
+1/-1     
NavigateCommand.cs
Refactor `NavigateResult` to inherit `BiDiResult`               
+1/-1     
PrintCommand.cs
Update `PrintResult` to inherit `BiDiResult`                         
+1/-1     
TraverseHistoryCommand.cs
Refactor `TraverseHistoryResult` to inherit `BiDiResult` 
+1/-1     
AddInterceptCommand.cs
Update `AddInterceptResult` to inherit `BiDiResult`           
+1/-1     
AddPreloadScriptCommand.cs
Refactor `AddPreloadScriptResult` to inherit `BiDiResult`
 +1/-1     
EvaluateCommand.cs
Update `EvaluateResult` hierarchy to inherit `BiDiResult`
 +1/-1     
GetRealmsCommand.cs
Refactor `GetRealmsResult` to inherit `BiDiResult`             
+1/-1     
NewCommand.cs
Update `NewResult` to inherit `BiDiResult`                             
+1/-1     
StatusCommand.cs
Refactor `StatusResult` to inherit `BiDiResult`                   
+1/-1     
SubscribeCommand.cs
Update `SubscribeResult` to inherit `BiDiResult`                 
+1/-1     
DeleteCookiesCommand.cs
Refactor `DeleteCookiesResult` to inherit `BiDiResult`     
+1/-1     
GetCookiesCommand.cs
Update `GetCookiesResult` to inherit `BiDiResult`               
+1/-1     
SetCookieCommand.cs
Refactor `SetCookieResult` to inherit `BiDiResult`             
+1/-1     
Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Apr 8, 2025
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Serialization

    The Write method for ElementOrigin is missing the actual serialization of the element.Element property. The code adds the property name but doesn't complete the serialization process.

     
    else if (value is ElementOrigin element)
{
    writer.WriteStartObject();
    writer.WriteString("type", "element");
    writer.WritePropertyName("element");
    JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>());
    writer.WriteEndObject();
}

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix serialization method call

    The code is using options.GetTypeInfo() which appears to be a custom extension
    method, but there's no indication that this method exists in the standard
    JsonSerializerOptions class. This could lead to runtime errors if the method is
    not properly defined or accessible.

    dotnet/src/webdriver/BiDi/Communication/Json/Converters/InputOriginConverter.cs [50-51]

     writer.WritePropertyName("element");
-JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>());
+JsonSerializer.Serialize(writer, element.Element, options);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a potential issue with the serialization method. The PR changed the serialization from using options directly to using options.GetTypeInfo<ISharedReference>(), which is a custom extension method that might not be properly defined or accessible, potentially causing runtime errors. Reverting to the original approach would be safer.

    		High
    Learned
    best practice
    Add null validation before accessing object properties to prevent potential NullReferenceExceptions

    The code is accessing element.Element without first checking if it's null. This
    could lead to a NullReferenceException if the Element property is null. Add a
    null check before serializing the element to prevent potential runtime
    exceptions.

    dotnet/src/webdriver/BiDi/Communication/Json/Converters/InputOriginConverter.cs [50-51]

     writer.WritePropertyName("element");
+ArgumentNullException.ThrowIfNull(element.Element, nameof(element.Element));
 JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>());
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • More

    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Apr 8, 2025

    Not conveyed in this PR: all the places where we keep the type as EmptyResult

    image

    @RenderMichael RenderMichael mentioned this pull request Apr 8, 2025
    Merged
    @nvborisenko
    Copy link
    Member

    nvborisenko commented Apr 8, 2025

    Related to #15562, let's discuss there our expectations.

    @nvborisenko
    Copy link
    Member

    nvborisenko commented Apr 8, 2025

    The EmptyResult does two different things: act as a base type for all BiDi results, and act as a void result.

    True, we will avoid user facing Task DoSomething() and rework to Task<Result> DoSomething() in #15562. If we agree, then separating BiDiResult and EmptyResult is unnecessary I think.

    @github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Oct 5, 2025
    @github-actions github-actions bot closed this Oct 19, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    C-dotnet .NET Bindings J-stale Applied to issues that become stale, and eventually closed. Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @RenderMichael @nvborisenko @selenium-ci